Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

implement .observe() #44

Merged
merged 3 commits into from
Apr 13, 2015
Merged

implement .observe() #44

merged 3 commits into from
Apr 13, 2015

Conversation

Rich-Harris
Copy link
Contributor

Thought I'd have a quick crack at something talked about previously on #31, #34 and #28 (comment) - giving each node an .observe() method that gets called when its input successfully builds, in order to do things like linting, style checking, tests, writing intermediate stages to disk for inspection, etc etc etc.

It doesn't make any progress towards the design outlined by @evs-chris in #28 (comment) - for example transformations aren't expressed in terms of observers, so there's a bit of duplication that would be unnecessary with a better architecture. My thinking is that we can hone the API through use first and improve the implementation later.

Example using gobble-eslint:

es5 = gobble( 'src' )
    .transform( 'babel', {
        whitelist: babelTransformWhitelist
    })
    .observe( 'eslint', {
        reportOnly: true // won't fail the build
    });

This lints code after it's been transformed by babel. That's useful because it means you can prevent things like IIFEs in for loops, which can easily happen if you use block binding, even if the original code is perfectly valid. It outputs something like this:

screen shot 2015-04-04 at 1 47 48 pm

Couple of points:

  • It's likely that some tasks would only be relevant in certain situations. At the moment, if the options object has __condition: false, then node.observe(fn, options) === node - i.e. it gets skipped. Example on eslint. Does that make sense, or is it a bit weird?
  • The ideal scenario would be that only changed files get linted (or whatever). At the moment that's tricky, because gobble's change detection is somewhat naive - if a sourcemap-generating file transformation gets re-run, all the files are deemed to have changed because the sourceMappingURL location gets bumped, even if the contents haven't changed.
  • On a related note, one of the proposed uses for this API is rerunning unit tests only when needed. Even if the change detection were more robust, this is tricky because we need to know which modules each test file depends on, and determine whether they have changed. This likely isn't a problem to solve.

Any feedback welcome!

@evs-chris
Copy link
Contributor

I like it!

The __condition on observers does feel a bit weird, but it doesn't seem like an reasonable stop-gap on the way to a better API. For the eslint example it seems to me that it would be better to have the user make that explicit. For instance, in my builds I usually have and if (gobble.env() === 'production') section that handles minification and whatnot, and that section usually has a complement for dev mode stuff. Having the build skip steps automatically based on how it's called without any other instruction from me directly seems a bit surprising. I do see the value of having it 'just do the right thing', where the 'right thing' is right in 95% of all cases too.

For rerunning select tests, I'd say leave that to the user too. There are too many different ways to set up source and tests to guess which tests need to be rerun when certain sources change. It might be nice to have a specialized observer for those that did a one-to-one module source folder to test folder, though.

+ src
|--+ foo
|--+ bar
|--+ baz
+ test
|--+ foo
|--+ bar
|--+ baz

gobble('src').observe('testModules', { tests: gobble('test') });

As you say though, that's not really a problem for the API to solve.

One thing I've run into with the server scenario is that the downstream transforms aren't notified when there's a failure, so it leaves the server running on a subsequent failed build. That's not necessarily a bad thing, but it can be a bit confusing if you don't keep an eye on your build output. It might be nice to allow downstream observers to get a failure notification so they can clean up and not leave stale or incorrect stuff sitting around if they need to do so.

@Rich-Harris
Copy link
Contributor Author

After sitting with it for a couple of days I reached the same conclusion - the __condition thing is weird as hell. Not sure what I was thinking. I reckon this could work though:

module.exports = gobble( 'src' )
  .transform( someTransformer, {...})
  .observeIf( someCondition, observer, {..})
  .transform( someOtherTransformer, {...});
  // ...

// equivalent to
var node = gobble( 'src' )
  .transform( someTransformer, {...});

if ( someCondition ) {
  node = node.observe( observer, {...});
}

module.exports = node
  .transform( someOtherObserver, {...});

This isn't such a contrived example - I often find myself storing temporary references so I can do things like...

if ( gobble.env() === 'production' ) {
  node = node.transform( 'uglifyjs' );
}

...which is basically the same thing. And a .transformIf(...) method would be a nice counterpart to .observeIf(...). Does that sound like a better solution?

It might be nice to allow downstream observers to get a failure notification

Yeah, I agree.

Rich-Harris added a commit that referenced this pull request Apr 13, 2015
@Rich-Harris Rich-Harris merged commit 133872a into master Apr 13, 2015
@evs-chris
Copy link
Contributor

👍

@Rich-Harris Rich-Harris deleted the observe branch April 13, 2015 19:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants